Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix mod for mixes of Signed and Unsigned #57853

Merged
merged 4 commits into from
Mar 23, 2025
Merged

fix mod for mixes of Signed and Unsigned #57853

merged 4 commits into from
Mar 23, 2025

Conversation

oscardssmith
Copy link
Member

Previously this was just overfowing producing wrong answers (both for the sign convention and just the wrong modulo class)
fixes #57851

@oscardssmith
fix mod for mixes of Signed and Unsigned
a3c4877 
Previously this was just overfowing producing wrong answers (both for the sign convention and just the wrong modulo class)
fixes #57851
@oscardssmith
add test
55e3362
@oscardssmith
simplify slightly
32713dd
@oscardssmith oscardssmith added bugfix This change fixes an existing bug backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 maths Mathematical functions correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing labels Mar 21, 2025
@StefanKarpinski
Copy link
Member

StefanKarpinski commented Mar 21, 2025 •
edited
Loading

It should be noted that this changes the return type in these mixed cases, but that does seem like a bug fix. Given that the sign of the result comes from the second argument, it only seems sensible that the signedness also comes from the second argument. In fact, it would be find if the result type was simply the type of the second argument.

@oscardssmith
Copy link
Member Author

This doesn't change the result type.

@KristofferC KristofferC mentioned this pull request Mar 22, 2025
Merged
@StefanKarpinski
Copy link
Member

Oh, nvm then.

@oscardssmith
Copy link
Member Author

(it actually did change the result type, but that was a bug)

@oscardssmith oscardssmith merged commit 0568917 into master Mar 23, 2025
7 checks passed
@oscardssmith oscardssmith deleted the os/fix-signed-unsigned-mod branch March 23, 2025 01:55
KristofferC pushed a commit that referenced this pull request Mar 24, 2025
@oscardssmith @KristofferC
fix mod for mixes of Signed and Unsigned (#57853)
78f810b 
Previously this was just overfowing producing wrong answers (both for
the sign convention and just the wrong modulo class)
fixes #57851

(cherry picked from commit 0568917)
@KristofferC KristofferC mentioned this pull request Mar 24, 2025
Open
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 bugfix This change fixes an existing bug correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing maths Mathematical functions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The Base functions mod(::Unsigned, ::Signed) and mod(::Signed, ::Unsigned) are broken
2 participants
@oscardssmith @StefanKarpinski